Indicator data input #245

matthiasschaub · 2022-02-03T10:24:38Z

Description

An Indicator consists of a Layer and a Feature.

This PR introduces a new Layer baseclass. Now a Layer object can be either an instance of a "Layer Definition" class (with information how to retrieve the data for the layer) or of a "Layer Data" class (with data already attached to itself). This way it is possible to compute an Indicator for given data. At the moment this is restricted to the MappingSaturation Indicator.

In particular the PR adds:

New Layer base-class and Layer sub-classes
New API parameter layer (containing name, description and data) to the endpoint indicator (POST)

Below is an example of the request body for a POST request to the /indicator endpoint.

{
  "name": "MappingSaturation",
  "bpolys": {
    "type": "Feature",
    "geometry": {
      "type": "Polygon",
      "coordinates": [
        [
          [
            8.674092292785645,
            49.40427147224242
          ],
          [
            8.695850372314453,
            49.40427147224242
          ],
          [
            8.695850372314453,
            49.415552187316095
          ],
          [
            8.674092292785645,
            49.415552187316095
          ],
          [
            8.674092292785645,
            49.40427147224242
          ]
        ]
      ]
    }
  },
  "layer": {
    "name": "",
    "description": "",
    "data": {
      "result": [
        {
          "value": 1,
          "timestamp": "2020-03-20T01:30:08.180856"
        },
        {
          "value": 56,
          "timestamp": "2020-03-20T01:30:08.180856"
        },
        {
          "value": 5581,
          "timestamp": "2020-03-20T01:30:08.180856"
        },
      ]
    }
  }
}

Corresponding issue

Closes #233

Checklist

joker234

First overview looks pretty good, but one question we have to clarify before it makes sense to review in more detail: Does it make sense to name the new parameter layer? I would assume it means the layer name and not layer data you can give the endpoint for calculation.

CHANGELOG.md

joker234

I'm generally not really of using **_kargs or *_args, especially if you don't use them. Isn't there a better solution?

workers/ohsome_quality_analyst/api/request_models.py

workers/ohsome_quality_analyst/indicators/currentness/indicator.py

workers/ohsome_quality_analyst/indicators/mapping_saturation/indicator.py

workers/ohsome_quality_analyst/ohsome/client.py

workers/ohsome_quality_analyst/oqt.py

workers/tests/integrationtests/test_api_indicator_geojson_io.py

workers/tests/unittests/mapping_saturation/test_indicator.py

workers/tests/unittests/test_ohsome_client.py

matthiasschaub · 2022-04-08T07:01:32Z

I'm generally not really of using **_kargs or *_args, especially if you don't use them. Isn't there a better solution?

Those are used because of flags like the force flag which is an argument to some of the overloaded functions and not to others. I wanted to signal that this is the case.

E.g. Notice the different second arguments:

@create_indicator_as_geojson.register(IndicatorBpolys)
@create_indicator_as_geojson.register(IndicatorData)
async def _create_indicator_as_geojson(
    parameters: Union[IndicatorBpolys, IndicatorData],
    size_restriction: bool = False,
    **_kargs,
):
    pass


@create_indicator_as_geojson.register
async def _create_indicator_as_geojson(
    parameters: IndicatorDatabase,
    force: bool = False,
    **_kargs,
):
    pass

I am totally open for another solution.

PS: The caller of the function create_indicator_as_geojson always passes down the force flag. Even in the case where it is not needed.

joker234

Thanks for the changes. Made only a few comments. If you want you can have a look at SonarCloud: https://sonarcloud.io/summary/new_code?id=ohsome-quality-analyst&pullRequest=245

workers/tests/unittests/test_ohsome_client.py

attached to itself. Factor out the layer classes from `base/indicator.py` to own module.

Instead of initializing an Indicator with a `layer_name` as string initialize an Indicator with a `layer` as Layer object.

Also first instantiate a Layer object and then pass it as init parameter to the Indicator object.

instead of making a request to the ohsome API to retrieve data Call ohsome.query function with args not kwargs.

Make usage of singledispatch more concise. Address review comments of the dispatch functions: - Adhere to convention to name overloaded func '_' - Remove # noqa comments - Rename 'kargs' to 'kwargs' - Add test cases for generic functions covering 'NotImplemented'

Ignore duplications in the tests fixtures.py Fix timestamp range in test fixtures Avoid loading the same fixture for each test case In test do not use empty string for layer name

of the API instead of using empty strings.

matthiasschaub added enhancement New feature or request waiting An issue or PR which is waiting for an upstream bugfix, further information or is somehow blocked labels Feb 3, 2022

matthiasschaub self-assigned this Feb 3, 2022

matthiasschaub force-pushed the indicator-data-input branch 2 times, most recently from c8bdc53 to eca4957 Compare February 3, 2022 12:55

matthiasschaub force-pushed the indicator-data-input branch 2 times, most recently from 4b3e6fa to eca4957 Compare February 11, 2022 11:04

matthiasschaub force-pushed the indicator-data-input branch 2 times, most recently from 1d776bf to 05e75e5 Compare March 9, 2022 07:30

matthiasschaub force-pushed the indicator-data-input branch 7 times, most recently from 1e89d10 to c8e2f11 Compare March 14, 2022 16:29

matthiasschaub marked this pull request as ready for review March 14, 2022 16:29

matthiasschaub requested a review from joker234 March 14, 2022 16:29

matthiasschaub force-pushed the indicator-data-input branch from c8e2f11 to 9123f87 Compare March 15, 2022 10:04

matthiasschaub marked this pull request as draft March 16, 2022 06:41

matthiasschaub marked this pull request as ready for review March 16, 2022 15:18

matthiasschaub mentioned this pull request Mar 17, 2022

Add support for GroupBy/boundaries queries to the ohsome API client #272

Merged

9 tasks

matthiasschaub removed the waiting An issue or PR which is waiting for an upstream bugfix, further information or is somehow blocked label Mar 17, 2022

GIScience deleted a comment from matthiasschaub Mar 22, 2022

joker234 reviewed Mar 22, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

matthiasschaub force-pushed the indicator-data-input branch 4 times, most recently from d393b69 to 711f7b0 Compare March 24, 2022 09:28

joker234 reviewed Apr 6, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

joker234 requested changes Apr 6, 2022

View reviewed changes

matthiasschaub force-pushed the indicator-data-input branch from 06ac588 to ce5c6b4 Compare April 7, 2022 06:53

matthiasschaub requested a review from joker234 April 8, 2022 08:59

joker234 reviewed Apr 11, 2022

View reviewed changes

joker234 previously approved these changes Apr 12, 2022

View reviewed changes

matthiasschaub dismissed joker234’s stale review via b818b41 April 12, 2022 12:59

matthiasschaub force-pushed the indicator-data-input branch from 32c23b9 to b818b41 Compare April 12, 2022 12:59

matthiasschaub and others added 11 commits April 12, 2022 14:59

Implement Layer dataclass for layers with data

3807ce7

attached to itself. Factor out the layer classes from `base/indicator.py` to own module.

Add request model for Layer with data attached

b2c9666

Indicators are initialized with a Layer object

4fcccdb

Instead of initializing an Indicator with a `layer_name` as string initialize an Indicator with a `layer` as Layer object.

Add function create_indicator using custom Layer

1ca9f28

Also first instantiate a Layer object and then pass it as init parameter to the Indicator object.

If Layer has data attached return data immediately

a445d66

instead of making a request to the ohsome API to retrieve data Call ohsome.query function with args not kwargs.

Move 'schema' from dev-dependency to dependency

7dc26fd

Validate ohsome response in 'ohsome/client' module

d082788

Add changes to the CHANGELOG

f7830f8

Minor formatting changes

d80785f

Various small improvements to the tests

b1ea2e4

Ignore duplications in the tests fixtures.py Fix timestamp range in test fixtures Avoid loading the same fixture for each test case In test do not use empty string for layer name

matthiasschaub force-pushed the indicator-data-input branch from b818b41 to b1ea2e4 Compare April 12, 2022 13:02

Add layer name and description to request examples

5662d55

of the API instead of using empty strings.

joker234 approved these changes Apr 12, 2022

View reviewed changes

matthiasschaub merged commit 3bd37b8 into main Apr 12, 2022

matthiasschaub deleted the indicator-data-input branch April 12, 2022 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indicator data input #245

Indicator data input #245

matthiasschaub commented Feb 3, 2022 •

edited

Loading

joker234 left a comment

joker234 left a comment

matthiasschaub commented Apr 8, 2022 •

edited

Loading

joker234 left a comment

Indicator data input #245

Indicator data input #245

Conversation

matthiasschaub commented Feb 3, 2022 • edited Loading

Description

Corresponding issue

Checklist

joker234 left a comment

Choose a reason for hiding this comment

joker234 left a comment

Choose a reason for hiding this comment

matthiasschaub commented Apr 8, 2022 • edited Loading

joker234 left a comment

Choose a reason for hiding this comment

matthiasschaub commented Feb 3, 2022 •

edited

Loading

matthiasschaub commented Apr 8, 2022 •

edited

Loading